Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add JSDoc example #8533

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

linonetwo
Copy link
Contributor

Just an example yet.

Copy link

Confirmed: linonetwo has already signed the Contributor License Agreement (see contributing.md)

Copy link
Member

@Jermolene Jermolene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @linonetwo

@@ -1,4 +1,5 @@
/*\
// @ts-check
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It enables VSCode and ts cli regard this file as .ts file. Maybe this enable different parse mode of VSCode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs seem to imply that one can also use a separate "jsconfig.json" file. Would that be possible here?

Copy link
Contributor Author

@linonetwo linonetwo Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try again and find VScode works with or without this. What make it work is open both file as tab...

I need to do more experiment.

@@ -1,4 +1,5 @@
/*\
// @ts-check
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to change the existing /*\ pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I find @module $:/core/modules/parsers/wikiparser/rules/codeblock.js is useless for us, so the comment here don't need to be JSDoc anymore.

Copy link
Contributor Author

@linonetwo linonetwo Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the comment for type needs to be valid jsdoc

Without JSDoc:

截屏2024-09-10 15 04 18

With JSDoc:

截屏2024-09-10 15 04 26

@@ -11,7 +12,28 @@ Wiki text rule for code blocks. For example:
```
```

@module $:/core/modules/parsers/wikiparser/rules/codeblock.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should extend the JS deserialiser to recognise @module as a synonym for title: ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not working, while this usage exists. I need to figure out why.

@pmario
Copy link
Member

pmario commented Sep 9, 2024

@linonetwo -- After your latest changes, does VSCode show useful type info in the tooltips now?

@linonetwo
Copy link
Contributor Author

截屏2024-09-10 10 22 59

@pmario Yes, I find // @ts-check is not necessary, but the tsconfig.json is necessary (And of cource also need the TypeScript plugin on your code editor).

I'm experiment with the tidddlywiki npm package, because this PR is intended for providing AST type for plugin developers.

@linonetwo
Copy link
Contributor Author

截屏2024-09-10 16 15 15

Now $tw.wiki.xxx have type check and docs.

@linonetwo
Copy link
Contributor Author

linonetwo commented Sep 10, 2024

Solution to fix core/modules/widgets/widget.js(9,1): error TS9005: Declaration emit for this file requires using private name 'Widget'. An explicit type annotation may unblock declaration emit.

change

var Widget = function(parseTreeNode,options) {
	this.initialise(parseTreeNode,options);
};

to

function Widget(parseTreeNode,options) {
	this.initialise(parseTreeNode,options);
};

and add @class in the jsdoc.


/**
* @type {Widget}
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the 2 comment blocks be merged to 1 block, or do we need 2 of them?

* @param {string} title - The title of the tiddler.
* @param {string} [defaultText] - The default text to return if the tiddler is missing.
* @returns {string | null | undefined} - The text of the tiddler, undefined if the tiddler is missing, or null if the tiddler is being lazily loaded.
*/
Copy link
Member

@pmario pmario Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the type text should be as short as possible, but still understandable. Your definition contains "undefined", which is not returned. So I would suggest:

/**
* Returns the tiddler text-field. If field `_is_skinny` is present it triggers a "lazyLoad" event.
* 
* @param {string} title - Tiddler title
* @param {string} [defaultText] - Returned if tiddler is missing
* @returns {string | null } - Returns the "text-field" or an empty string, "defaultText" if the tiddler is missing or null if the tiddler is being lazily loaded
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When add @returns, it means it always return, so can't remove undefined here. It is same in TypeScript.

* @property {number} attributes.language.start - The start position of the language string in the source text.
* @property {number} attributes.language.end - The end position of the language string in the source text.
*/

Copy link
Member

@pmario pmario Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO having @property {string} type and then - The type of the widget, which is "codeblock". is the same thing. Both elements describe: "What it is". -- I think the second part should describe: "What it does", otherwise we can skip it.

  • @Property does describe -- What it is
  • Text describes -- What it does

eg:

/**
 * Represents the parser `codeblock` rule.
 * 
 * @typedef {Object} CodeblockNode
 * @property {string} rule - Always "codeblock"
 * @property {string} type - Always "codeblock"
 * @property {number} start - Rule start marker in source text
 * @property {number} end - Rule end marker in source text
 * @property {Object} attributes - Contains "code" and "language" objects
 * @property {Object} attributes.code - The code attribute object
 * @property {string} attributes.code.type - Always "string"
 * @property {string} attributes.code.value - Actual code
 * @property {number} attributes.code.start - Start position of code in source text
 * @property {number} attributes.code.end - End position of code in source text
 * @property {Object} attributes.language - The language attribute object
 * @property {string} attributes.language.type - Always "string"
 * @property {string} attributes.language.value - Language specified after the triple backticks, if any
 * @property {number} attributes.language.start - Start position of the string in source text
 * @property {number} attributes.language.end - End position of the string in source text
 */

Should be as short as possible and still valid. Especially see type, rule, start, end which are properties of "CodeblockNode"

I did remove the "full stops" from all @ elements. They are not needed
The first intro sentence has a full stop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'd copy that.

I have to admit, all jsdoc are generated by github copilot. I don't have time for this detail, I will leave it for future refinement. I'm still debugging the type when I'm using it in my plugin project.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit, all jsdoc are generated by github copilot. I don't have time for this detail, I will leave it for future refinement. I'm still debugging the type when I'm using it in my plugin project.

The new type information will significantly increase the TW code-size. So if there is redundant information it should be removed.

And even more important it has to be valid. So if the co-pilot only adds comments in a more verbose form than the parameters are. There should not be any comments at all -- They have no value.

So if there are no comments, we actually know, that we have to add them manually. IMO for the tooltips it would be OK to start with the type info.

Copy link
Contributor Author

@linonetwo linonetwo Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will significantly increase the TW code-size

we must remove comments before publishing HTML wiki. I think these JSDoc will basically double the size.

it has to be valid

This won't be a big problem, because 1. I use the method body as input too. And 2. we can auto merge "comment" type of PR based on #7542 , so people can update comment quickly. I find this is quite frequent when maintaining https://github.com/tiddly-gittly/TW5-Typed

@linonetwo
Copy link
Contributor Author

Plugin developers can install typescript globally, and run tsc or rm -rf ./types/generated && tsc > log.log in ./node_modules/tiddlywiki of their plugin project, to generate type that can be used in the plugin.

But I'm still working on this, some imported type is not working currently.

* @param {boolean} [options.parseAsInline=false] - If true, the text will be parsed as an inline run.
* @param {Object} options.wiki - Reference to the wiki to use.
* @param {string} [options._canonical_uri] - Optional URI of the content if the text is missing or empty.
* @param {boolean} [options.configTrimWhiteSpace=false] - If true, trims white space according to configuration.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the full-stop at the end of the comment text can be removed.
IMO "The" and "the" can be removed -- most of the time. So it does not obscure the important content. eg:

 * @param {string} type - Content type of the text to be parsed
 * @param {string} text - Text to be parsed
 * @param {Object} options - Parser options
 * @param {boolean} [options.parseAsInline=false] - If true, the text will be parsed as an inline run
 * @param {Object} options.wiki - Reference to the wiki store in use
 * @param {string} [options._canonical_uri] - Optional URI of the content if text is missing or empty
 * @param {boolean} [options.configTrimWhiteSpace=false] - If true, the parser trims white space

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this will be the updated prompt:

add jsdoc, only output jsdoc for this class and its method, no impl. full-stop at the end of the comment text can be removed. "The" and "the" can be removed -- most of the time. So it does not obscure the important content.

@linonetwo
Copy link
Contributor Author

Now plugin developers can get type hint directly generated from TW source code:

1. tsconfig.json in plugin root

As a plugin developer, in your project:

  "compilerOptions": {
    "paths": {
      // Allow `import('$:/core/modules/...')` instead of `import('tiddlywiki/core/modules/...')`. Only works inside this project.
      "$:/core/*": ["node_modules/tiddlywiki/types/core/*"],
      "tiddlywiki/*": ["node_modules/tiddlywiki/types/*"],
    },

Don't need allowJs checkJs, because we are going to...

2. run tsc in node_modules/tiddlywiki/

Hope @Jermolene can do this before publish, otherwise we have to do it by ourself.

This will generate types/core/modules/ folder full of .d.ts file. So we can do

import type { ParseTreeNode } from 'tiddlywiki/types/core/modules/parsers/base';

I find it is impossible to directly import JSDoc from 'tiddlywiki/core/modules/parsers/base'; (without /types/), even open maxNodeModuleJsDepth in tsconfig.

3. enjoy the type

截屏2024-09-12 17 52 55

4. maintaining the type

I only add typing to some typical AST node in this PR. Also I enable type of $tw.wiki.xxx to show this can be helpful not only to plugin developers, but also to core developers.

Now I need to know your opinion before I spend more 10h+ time to this.

@linonetwo linonetwo marked this pull request as ready for review September 14, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants